Faster entity cloning #16717
Faster entity cloning #16717
Conversation
(for types with ReflectDefault only)
alice-i-cecile
added
A-ECS
|
Fixed tests for reflect-based cloning paths not actually checking reflect paths (oops) and |
|
@JaySpruce can I get your review here please? |
Thankfully this should be totally fine since the derive macro always registers it with no way to opt out :) |
| if self | ||
| .components | ||
| .component_id::<T>() | ||
| .is_some_and(|id| id == self.component_id) |
There was a problem hiding this comment.
It seems unfortunate to have to look up the component_id again when we already have it, but I guess it would be hard to pass along the safety requirements, and it's not a very expensive lookup.
This also means it can't work with dynamic components. It might be worth doing the lookup the other way, getting the ComponentDescriptor for self.component_id and checking that type_id() == Some(TypeId::of::<T>()). I don't actually know whether dynamic components based on a rust type will have a type_id(), though, so it might not matter.
There was a problem hiding this comment.
This also means it can't work with dynamic components.
I think that we can probably allow to clone dynamic components by exposing some unsafe methods on ComponentCloneCtx to get source component Ptr and to copy from provided Ptr to target. This would let someone who created dynamic component to register their own component clone handler.
| let source_type_id = self | ||
| .components | ||
| .get_info(self.component_id) | ||
| .unwrap() |
There was a problem hiding this comment.
These unwrap()s should probably be expect()s so that we can give more helpful error messages.
| } | ||
| let source_type_id = self | ||
| .components | ||
| .get_info(self.component_id) |
There was a problem hiding this comment.
This could use the component_info variable from a few lines later to save one lookup.
For that matter, it might be worth doing the lookup ahead of time and storing the ComponentInfo in the context. That avoids having to look it up in both the read_source and write_target methods. You aren't currently doing that lookup in the non-reflect versions, but you could replace the self.components.component_id::<T>() == self.component_id checks with component_info.type_id() == Some(TypeId::of::<T>()) and wind up with a little less code overall.
| .unwrap() | ||
| .type_id() | ||
| .unwrap(); | ||
| let component_type_id = component.reflect_type_info().type_id(); |
There was a problem hiding this comment.
Can you use component.type_id() here? Any is a supertrait of Reflect, so I think it's available.
The reflect_type_info() method delegates to one on trait Typed, which is a safe trait. I believe a malicious implementation could return the wrong type, and then the copy_nonoverlapping() call would be UB. But Any is implemented by the compiler and can be relied upon.
| (handler)(&mut world.into(), self); | ||
|
|
||
| // SAFETY: There are no other mutable references to source entity. | ||
| let Some(source_component_ptr) = (unsafe { source_entity.get_by_id(component) }) else { |
There was a problem hiding this comment.
The entity is guaranteed to have this component because it was part of its own archetype, right? It might be more clear to use debug_checked_unwrap() so that it's obvious get_by_id never fails.
| // - component is a valid value represented by component_id | ||
| unsafe { | ||
| let raw_component = | ||
| core::ptr::NonNull::new_unchecked(Box::into_raw(component).cast::<u8>()); |
There was a problem hiding this comment.
Does this box get deallocated somewhere? The component is moved out by insert_by_id, but I think the allocation is leaked.
There was a problem hiding this comment.
It is leaked according to miri, good catch.
| let registry = self.type_registry?.read(); | ||
| let type_id = self.components.get_info(self.component_id)?.type_id()?; | ||
| let reflect_from_ptr = registry.get_type_data::<bevy_reflect::ReflectFromPtr>(type_id)?; | ||
| // SAFETY: `source_component_ptr` stores data represented by `component_id`, which we used to get `ReflectFromPtr`. |
There was a problem hiding this comment.
I think you need to check that reflect_from_ptr.type_id() == type_id(). A malicious client could insert the ReflectFromPtr for the wrong type by calling registry.get_mut(TypeId::of::<A>()).unwrap().insert(<ReflectFromPtr as FromType<B>>::from_type()), and then the as_reflect() call would be UB.
…tr`'s `TypeId` does not match component's `TypeId`
It should never fail because `component` is part of entity's archetype
…target_component_reflect`
…dlers for components without `TypeId`
|
I think this should address everything outlined by reviews at the moment. Also added a method |
|
@eugineerd I agree with the last two suggestions; ping me when those are addressed and I'll get this merged for you! |
…` inside `ComponentCloneCtx` Co-authored-by: Chris Russell <[email protected]>
Co-authored-by: Chris Russell <[email protected]>
|
@alice-i-cecile applied suggestions and CI is green now. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Needs a bit of cleanup to make sure this builds okay with |
|
@alice-i-cecile fixed |
Objective
#16132 introduced entity cloning functionality, and while it works and is useful, it can be made faster. This is the promised follow-up to improve performance.
Solution
PREFACE: This is my first time writing
unsafein rust and I have only vague idea about what I'm doing. I would encourage reviewers to scrutinizeunsafeparts in particular.The solution is to clone component data to an intermediate buffer and use
EntityWorldMut::insert_by_idsto insert components without additional archetype moves.To facilitate this,
EntityCloner::clone_entitynow reads all components of the source entity and provides clone handlers with the ability to read component data straight from component storage usingread_source_componentand write to an intermediate buffer usingwrite_target_component.ComponentIdis used to check that requested type corresponds to the type available on source entity.Reflect-based handler is a little trickier to pull of: we only have
&dyn Reflectand no direct access to the underlying data.ReflectFromPtrcan be used to get&dyn Reflectfrom concrete component data, but to write it we need to create a clone of the underlying data usingReflect. For this reason only components that haveReflectDefaultorReflectFromReflectorReflectFromWorldcan be cloned, all other components will be skipped. The good news is that this is actually only a temporary limitation: once #13432 lands we will be able to clone component without requiring one of thesetype datas.This PR also introduces
entity_cloningbenchmark to better compare changes between the PR and main, you can see the results in the showcase section.Testing
Showcase
Here's a table demonstrating the improvement:
*: these benchmarks have entities with only 1 component
Considerations
Once #10154 is resolved a large part of the functionality in this PR will probably become obsolete. It might still be a little bit faster than using command batching, but the complexity might not be worth it.
Migration Guide
&EntityClonerin component clone handlers is changed to&mut ComponentCloneCtxto better separate data.EntityCloneHandlerfrom enum to struct and added convenience functions to add default clone and reflect handler more easily.